-
Notifications
You must be signed in to change notification settings - Fork 44
Create plugin for Environment Tab in LC #2120
Conversation
Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
Can one of the admins verify this patch? |
close #2119 |
testPR |
Signed-off-by: Oleksii Korniienko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also check all buttons, if they are opening new shell/dialog and check, if this dialog is opened
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
* | ||
* @param Name variable name | ||
*/ | ||
public void select(String Name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to use better name: like selectVariable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
...eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentLaunchConfigurationTab.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Oleksii Korniienko <[email protected]>
/** | ||
* Add new environment variable | ||
* | ||
* @param Name variable name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also update the javadoc after changing variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
...er.eclipse/src/org/eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentTab.java
Show resolved
Hide resolved
...er.eclipse/src/org/eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentTab.java
Outdated
Show resolved
Hide resolved
* | ||
* @param id variable index | ||
*/ | ||
public void select(int id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also add methods for selecting variables in EnvironmentTab? you have method getAllVariables, but how do I select one variable by name in this tab? Can you please also change name of current methods select(...) to selectEnvironmentVariable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
variables, added check for overwrite variable Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
edit, remove and copy Signed-off-by: Oleksii Korniienko <[email protected]>
new OkButton().click(); | ||
try { | ||
new WaitUntil(new ShellIsAvailable(OVERWRITE_SHELL_TITLE), TimePeriod.SHORT); | ||
new YesButton().click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overwriting value should be disabled by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that overwriting the value should be default choice but the option not to override is also implemented in api. I can imagine that user would like to add the intended value no matter there already exists one as he should know what he wants. Instead of not passing it be default which would require a debugging session to find out why the env is not set, imho.
* @param variableName variable name | ||
* @param newValue new variable value | ||
*/ | ||
public void edit(String variableName, String newValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can call edit(String variableName,String variableName,String newValue) or you can put null instead of second parameter and change edit(String variableName, String newName, String newValue) accordingly(if newName is null, do not change Name text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
param. Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
...er.eclipse/src/org/eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentTab.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
Signed-off-by: Oleksii Korniienko <[email protected]>
...er.eclipse/src/org/eclipse/reddeer/eclipse/debug/ui/launchConfigurations/EnvironmentTab.java
Outdated
Show resolved
Hide resolved
* @param name variable name | ||
* @param value variable value | ||
*/ | ||
public void add(String name, String nalue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can instead of basically rewriting the method that follows with boolean params, simply call the later implementation with default boolean value. And I would go with default overwrite = true instead of false...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @param variableName variable name for select | ||
*/ | ||
public void selectEnvironmentVariable(String variableName) { | ||
new PushButton("Select...").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have this similarities in implementation write only once. Like maybe using functional interface to pass specific code into repeated passages? Ie:
public void versionOne(String param) {
versionImpl(() -> {
System.out.println("I am String!");
});
}
public void versionTwo(int param) {
versionImpl(() -> {
System.out.println("I am int!");
});
}
private void versionImpl(Runnable mySpecificImpl) {
System.out.println("Actions before my functional interface call");
mySpecificImpl.run();
System.out.println("Actions after my functional interface call");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Oleksii Korniienko <[email protected]>
@jkopriva thanks for review, I would like to have this merged in M3 milestone, if there will be something you do not like, we would have it addressed after that. |
eclipse-archived/reddeer#2120 Signed-off-by: Oleksii Korniienko <[email protected]>
eclipse-archived/reddeer#2120 Signed-off-by: Oleksii Korniienko <[email protected]>
eclipse-archived/reddeer#2120 Signed-off-by: Oleksii Korniienko <[email protected]>
…#145) * JBIDE-27821: Created test for Environment variables in Quarkus Launch Configuration Signed-off-by: Oleksii Korniienko <[email protected]> * JBIDE-27821: fixed Copyright year Signed-off-by: Oleksii Korniienko <[email protected]> * JBIDE-27821: reworked "addEnvironment" due to eclipse-archived/reddeer#2120 Signed-off-by: Oleksii Korniienko <[email protected]> * JBIDE-27821: renamed class Signed-off-by: Oleksii Korniienko <[email protected]>
Pull Request Checklist
Code
Testing